Codex review: needs maintainer review before merge. Reviewed August 2, 2026, 2:20 PM ET / 18:20 UTC.

ClawSweeper review

What this changes

The branch maps Root.remove() target deletion errors to not-found, not-empty, or not-removable, while keeping parent-directory guard failures as existing boundary errors.

Merge readiness

⚠️ Ready for maintainer review - 4 items remain

This PR is a focused correction to the documented remove() error contract and its revised guard boundary preserves fail-closed handling. The remaining merge question is intentional compatibility acceptance: callers that caught the previous path-alias result for routine deletion failures will now receive the documented operational codes.

Priority: P2
Reviewed head: e96eb9c4b267f391c79855f0fec68b56517bfe45
Owner decision: Required. See Decision needed.

Review scores

Verification

How this fits together

Root.remove() deletes a path through a capability-style root, first resolving it under the root and pinning the parent directory’s identity. The parent guard protects confinement; only after it passes does the fallback inspect and delete the requested target, returning a public error code to the caller.

flowchart TD
  A[Caller requests removal] --> B[Resolve target under root]
  B --> C[Pin parent directory identity]
  C --> D{Parent guard succeeds?}
  D -->|No| E[Fail-closed boundary error]
  D -->|Yes| F[Inspect and remove target]
  F --> G[Map target syscall failure]
  G --> H[Public remove result]

Decision needed

Why: The patch is mechanically narrow and preserves the guard fail-closed boundary, but selecting which externally observable error contract takes precedence is a maintainer compatibility decision.

Before merge

• Resolve merge risk (P1) - Merging deliberately changes an existing public runtime outcome: consumers that catch path-alias around remove() for missing or non-empty targets will instead receive not-found or not-empty.
• Resolve merge risk (P1) - This code sits on a filesystem-confinement boundary; any future widening of deletion errno mapping beyond target syscalls could misreport an identity or guard failure as an ordinary removal outcome.
• Complete next step (P2) - No mechanical blocker remains; a root-confinement owner should explicitly accept the public error-code compatibility change before merge.

The two Windows check legs are red. I looked into it before assuming it was mine, and it is a pre-existing flake rather than a regression from this branch.

What failed here:

FAIL test/new-primitives.test.ts > file locks
     > supports manager lifecycle and top-level withFileLock

That exact test also fails on main. It went red in the release: 0.5.1 (#80) run, alongside file-lock-reentrancy.test.ts > continues to arbitrate with a separate process.

test/fs-safe.test.ts, which carries this PR's regression test, passed on the same Windows leg (26 tests | 12 skipped).

ci.yml history on main, most recent first:

A different test fails almost every run, across both Windows and ubuntu, and all of them are concurrency or file-handle timing cases. On my Windows machine file-lock-reentrancy.test.ts failed once with EPERM on a .lock file and then passed 4/4 in isolation, which matches the same pattern.

This change cannot reach that code: normalizeRemovePathError() is only called from removePathInRoot(), and src/sidecar-lock.ts / src/file-lock.ts do not import anything from src/root-errors.ts. The green legs on this PR (ubuntu 22/24, macOS 22/24, all native checks, CodeQL, package smoke) exercise the new code paths.

I cannot re-run the job myself — that needs repository admin rights — so if you would like a clean run before reviewing, a maintainer re-run or a rebase once main is green would both work; happy to do the rebase.

If the flakes are worth chasing separately, I am glad to open an issue with the run links collected above rather than expanding this PR.

Pushed e96eb9c for the guard-boundary finding. It is real: the errno mapping was applied to the whole fallback, and the parent-directory guard runs inside it.

What was wrong

removePathFallback() creates the guard, fires the test hook, and asserts the guard before it touches the target:

createAsyncDirectoryGuard(dirname)   <- lstat + realpath
assertAsyncDirectoryGuard(guard)     <- lstat + realpath
lstat(target) -> rmdir | rm          <- the only deletion

createAsyncDirectoryGuard() and assertAsyncDirectoryGuard() only convert known unsafe states into FsSafeError (not-file, path-mismatch). Any other raw errno from those lstat/realpath calls escaped, and with one outer normalizeRemovePathError() it became not-removable — reporting a failed deletion when no deletion was attempted, and losing the fail-closed reading of an unresolved identity condition.

What changed

The guard stage moved into prepareRemoveGuard() with its own normalizer, and the errno mapping now wraps only the deletion calls. removePathInRoot() is back to normalizePinnedPathError(), unchanged from main; everything below it already throws FsSafeError, so it is a pass-through.

normalizeRemoveGuardError():

The ENOENT/ENOTDIR carve-out is the one place the guard stage still maps to a documented code. It cannot mask an identity condition: identity drift and non-directory components already arrive as FsSafeError and return unchanged above that branch. It is also what keeps remove("missing-dir/missing.txt") on the documented not-found rather than reporting a boundary violation for an ordinary missing parent.

Evidence

The new end-to-end regression throws an errno-shaped ELOOP from the guard stage. On the previous head:

FAIL  test/edge-coverage.test.ts > root error helpers >
      does not map a guard-stage filesystem failure to a removal code
AssertionError: expected FsSafeError: path could not be removed { …(3) } to match
object { code: 'path-alias' }
- Expected
+ Received
- {
-   "code": "path-alias",
+ FsSafeError {
+   "code": "not-removable",

That is the finding reproduced: a guard-originated raw error surfacing as not-removable. After the fix it is path-alias, and the test also asserts the target file is still on disk.

Coverage added to test/edge-coverage.test.ts:

• the end-to-end case above, driven through the existing beforeRootFallbackMutation hook so it is deterministic on all six CI legs rather than needing a real ELOOP/EACCES, which is unforgeable as root in container CI and on Windows;
• unit assertions over normalizeRemoveGuardError() — FsSafeError pass-through, ENOENT/ENOTDIR to not-found, and ELOOP/EACCES/EBUSY/ENOTEMPTY/EEXIST plus a non-errno throw all to path-alias. ENOTEMPTY and EEXIST are in that list on purpose: they are removal outcomes, so from the guard stage they are nonsense and must fail closed rather than be reported as not-empty.

CHANGELOG.md now states the boundary alongside the code change.

Validation on Windows 11, Node v22.20.0:

lint:file-size    pass
lint:fs-boundary  pass
tsc --noEmit      pass
vitest run        Test Files  54 passed | 6 skipped (60)
                  Tests  491 passed | 183 skipped (674)
test:security     Test Files  5 passed (5)
                  Tests  46 passed | 17 skipped (63)

The pre-existing Windows lock flakes noted earlier did not reproduce in this run.

The red Windows leg here isn't from this diff — it's the flake family that's been on main since #79, and a re-run moved it.

I closed/reopened this PR to retrigger CI (I can't gh run rerun without admin). On identical code, the failing test changed:

Each time the other Windows Node version passed. A real defect fails the same leg every run; this one rotates.

It's also outside this PR's call graph. The diff touches src/root-errors.ts and src/root-impl.ts (remove() errno mapping only). movePathWithCopyFallback lives in src/move-path.ts, and file-lock-reentrancy doesn't reach either changed file.

For context on the first failure specifically — I did chase it before the re-run, in case it was a genuine identity bug. It isn't: with sourceHardlinks: "reject" the copy path stages .fs-safe-move-*.tmp while the source is still alive, so the staged file can't be handed the source's file ID, and copyEntryWithManifest bottoms out in copyRegularFilePinned with no link/clone path. I also ran the exact scenario 200× on a Windows box: 0 collisions. So the ino assertion looks sound and the failure is timing/environment, not semantics.

The dispatch job failure is unrelated infra — the ClawSweeper bot installation hit its own GitHub API rate limit (installation ID 122230863, HTTP 403), triggered by the pull_request_target hook on my reopen.

Happy to rebase once main is green if you'd prefer a clean run before merging.

Correction to my note above: I called the lock failures timing/environment. I chased them properly afterwards and they are a real, fixable defect — #87 has the root cause and the fix.

Windows denies access to a lock file while a just-unlinked directory entry is still being torn down, so a contended acquire() gets EPERM (errno -4048) on a name that is already gone. Instrumented at the moment of failure:

EPERM-DIAG {"attempt":2,"lstat":"ENOENT","siblings":["state.json"],"retryAfterMs":0}

lstat says ENOENT and a zero-delay retry opens the file. acquire() only treated EEXIST as contention, so the EPERM escaped from the exclusive create and from readSidecarLockSnapshot(), which maps only ENOENT to a vanished lock.

Two things I had wrong:

• It predates chore(deps): refresh runtime and build dependencies #79. src/sidecar-lock.ts last changed in fix(security): harden cross-platform containment #65; the dependency refresh only shifted timing enough to expose it.
• The rotation was a symptom of a real race, not proof of an environment problem. It rotates because the window is timing-dependent, not because nothing is broken.

Measured on Windows 11 / Node 22 over file-lock-reentrancy + new-primitives: 8 failures in 85 runs before, 0 in 110 after.

That covers the file-lock-reentrancy and new-primitives > file locks legs. The move-path-regression > publishes a fresh inode failure is a different symptom and still open — my analysis of that one in the comment above still stands as far as I took it.

This PR's own diff is unrelated to any of it, so #87 is independent of #84.

Landed as 6af7f93 via #93 — thank you, and your commits are credited on the squash.

Your diagnosis and your second push were both right. removePathInRoot() collapsed every non-FsSafeError into path-alias, so the three documented codes were declared in the exported union, documented in six places, and constructed nowhere. Separating the guard stage in prepareRemoveGuard() so a raw ELOOP is not reported as a removal outcome was a genuinely good catch — that is the kind of thing that usually only turns up after the fix ships.

I added one thing before landing. src/errors.ts defines OPERATIONAL_CODES, and not-found, not-empty and not-removable were absent from it, so categorizeFsSafeError() was still giving all three category: "policy" — the category consumers read as "a safety policy rejected this". Since the whole point of the change is that a routine ENOTEMPTY is not a boundary violation, fixing the code without the category would only have got you half the way there. All three are now operational, and docs/types.md states the rule.

Worth knowing for the downstream bump: this breaks src/infra/fs-safe-remove.test.ts:66-74 in openclaw/openclaw, which asserts /ENOTEMPTY|EEXIST|EPERM/; it makes findPathAliasFilesystemCause() dead code, since that helper existed purely to unwrap the errno from the bogus path-alias; and it quietly fixes three latent bugs there where not-found and not-removable checks were unreachable.

The red Windows legs on this PR were never yours — that was the sidecar-lock teardown race, fixed separately in #92.